Skip to content

docs: update readme from mariadb with port binding, tags, remove double spaces and informations #2552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

felipementel
Copy link
Contributor

@felipementel felipementel commented Mar 6, 2025

This pull request includes updates to the mariadb documentation and configuration files to improve clarity and compatibility, as well as to specify versions for certain images.

Documentation improvements:

  • mariadb/content.md: Added an important note about the %%IMAGE%% tags to highlight the latest and lts versions.
  • mariadb/content.md: Updated the Docker compose command to use docker compose instead of docker-compose for consistency with the latest Docker CLI.
  • mariadb/content.md: Added a section on port binding to explain how to expose the container port to the host port using the -p argument in the docker run command.

Configuration updates:

  • mariadb/stack.yml: Specified the image versions for mariadb and adminer to ensure consistency and compatibility. The mariadb image is updated to 11.7-ubi and adminer to 4.17.1.
  • mariadb/stack.yml: Remove docker compose version on file header - the docker compose version is obsolete

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few notes 👍

I'll also explicitly CC the mariadb image maintainers: @grooverdan @dbart @fauust

@@ -1,16 +1,17 @@
# Use root/example as user/password credentials
version: '3.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we drop this, we also need to drop the references to docker stack deploy; see also #2516 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we drop this, we also need to drop the references to docker stack deploy; see also #2516 (comment)

https://docs.docker.com/reference/compose-file/version-and-name/#version-top-level-element-obsolete

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more explicitly:

https://docs.docker.com/engine/swarm/stack-deploy "The docker stack deploy command uses the legacy Compose file version 3 format, used by Compose V1"

https://docs.docker.com/compose/intro/history/ " (from image - Compose V1 / Compose file version - "Used version top level element". So if you remove version, remove the docker stack deploy like @tianon said.

Comment on lines 6 to 14
image: mariadb:11.7-ubi
restart: always
environment:
MARIADB_ROOT_PASSWORD: example
ports:
- 3306:3306

adminer:
image: adminer
image: adminer:4.17.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These really specific version numbers are likely to bitrot quickly, so we don't typically suggest being so particular unless it's actually necessary to be.

(Also, see the prior note about exposed database ports.)

Copy link
Contributor

@grooverdan grooverdan Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially with 11.7 as a short term support release. Perhaps use a lts /lts-ubi tag and I guess something for adminer. The purpose of compose files was self contained, so especially with something link adminer, exposing ports seems unnecessary.

On env variables including something like MARIADB_AUTO_UPGRADE=1 would smooth the upgrade paths.

I wouldn't mind seeing the healthcheck and service dependency like https://mariadb.com/kb/en/using-healthcheck-sh/#compose-file-example here as a practically of showing of getting the database up before the services that connect to it (and might fail if they can't connect). (edit: assuming docker stack compatibility).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These really specific version numbers are likely to bitrot quickly, so we don't typically suggest being so particular unless it's actually necessary to be.

(Also, see the prior note about exposed database ports.)

updated.

especially with 11.7 as a short term support release. Perhaps use a lts /lts-ubi tag and I guess something for adminer. The purpose of compose files was self contained, so especially with something link adminer, exposing ports seems unnecessary.

On env variables including something like MARIADB_AUTO_UPGRADE=1 would smooth the upgrade paths.

I wouldn't mind seeing the healthcheck and service dependency like https://mariadb.com/kb/en/using-healthcheck-sh/#compose-file-example here as a practically of showing of getting the database up before the services that connect to it (and might fail if they can't connect). (edit: assuming docker stack compatibility).

I like the idea.

I think another section, with this more complete example, would be better. Then we can have a more basic section and a more advanced one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thread isn't resolved (references in stack.yml are still too-specific).

Copy link
Contributor

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the cleanup.

@felipementel
Copy link
Contributor Author

@tianon, @grooverdan
There are other points to yours check?

felipementel

This comment was marked as duplicate.

Copy link
Contributor Author

@felipementel felipementel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to make any further adjustments for analysis and approval?

@grooverdan
Copy link
Contributor

sorry for delay. I'll get to it next week.

@felipementel
Copy link
Contributor Author

@grooverdan
What can I do to merge this pull request?

Copy link
Contributor

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous comments remain unaddressed.

@@ -1,16 +1,17 @@
# Use root/example as user/password credentials
version: '3.1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more explicitly:

https://docs.docker.com/engine/swarm/stack-deploy "The docker stack deploy command uses the legacy Compose file version 3 format, used by Compose V1"

https://docs.docker.com/compose/intro/history/ " (from image - Compose V1 / Compose file version - "Used version top level element". So if you remove version, remove the docker stack deploy like @tianon said.

@felipementel
Copy link
Contributor Author

previous comments remain unaddressed.

I update the both tags to latest

@felipementel felipementel requested a review from grooverdan April 25, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants